Skip to content

bun:test: remove the dead toAsymmetricMatcher print hook for custom matchers - #40050

Open
robobun wants to merge 1 commit into
mainfrom
farm/fbb9d38b/custom-asymmetric-matcher-print
Open

robobun wants to merge 1 commit into
mainfrom
farm/fbb9d38b/custom-asymmetric-matcher-print

Conversation

@robobun

@robobun robobun commented Aug 22, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • When a matcher registered with expect.extend carries a toAsymmetricMatcher property, a failing toEqual prints the custom asymmetric matcher as "n": [object Object]toBeDivisibleBy [ 3, ].
  • ExpectCustomAsymmetricMatcher::custom_print (src/runtime/test_runner/expect.rs:2681, removed here) looked up that property, then called the matcher itself in its place. The matcher returns { pass, message }, which stringifies to [object Object]. It then returned false, so the caller at src/runtime/test_runner/pretty_format.rs:2911 printed the default <name> [args] form as well.
  • The Zig original (ExpectCustomAsymmetricMatcher.customPrint, feat(test): expect.extend and asymmetric matcher improvements #7319) had the same two defects. The hook has never produced correct output, and it has no types, no docs, and no Jest or Vitest counterpart.

Fix

  • Delete custom_print and its helper maybe_clear. The formatter prints the default form directly. The diff line becomes "n": toBeDivisibleBy [ 3, ], with the not and promise prefixes intact.
  • Correct because a toAsymmetricMatcher property on the matcher function is ignored in Jest too: Jest and Vitest put the print form on the matcher instance, and expect.extend offers no user hook for it. The hook is dead native code with no user-visible behavior to keep. The repo guidance asks new user-reachable surface to ship complete (types, docs) or not at all, and a review of the first version of this PR found that enabling the hook as written would print expect.not.x(3) and expect.x(3) identically.
  • This also removes a user JS re-entry from inside the formatter and an .expect("unreachable") around it.
  • Verified: test/js/bun/test/expect-extend-asymmetric-print.test.ts (new file, one test, fails on stock bun 1.4.0). Also ran expect.test.js, jest-extended.test.js, expect-extend-*.test.ts, pretty-format-overflow.test.ts, expect/huge-failure-message.test.ts, and the three test/regression/issue files that use expect.extend.

Background

  • An asymmetric matcher is a value like expect.any(Number) that toEqual matches by a predicate. expect.toBeDivisibleBy(3) creates one for a custom matcher. Bun stores the matcher function and the captured args on a native ExpectCustomAsymmetricMatcher instance.
  • When a diff or this.utils.stringify prints such an instance, pretty_format.rs writes the promise prefix, not if negated, the matcher name, and the captured args array.
  • Jest prints a custom asymmetric matcher as [not.]name<args> from a toAsymmetricMatcher() method that Jest itself generates on the instance. Bun's default form is [not ]name [args]. This PR does not change that form.
Notes
  • Repro on bun 1.4.0-canary.1 (4448a2e) and on main at b30c6ff:

    import { expect, test } from "bun:test";
    function toBeDivisibleBy(actual: unknown, divisor: number) {
      const pass = typeof actual === "number" && actual % divisor === 0;
      return { pass, message: () => `expected ${actual} to be divisible by ${divisor}` };
    }
    (toBeDivisibleBy as any).toAsymmetricMatcher = (divisor: number) => `DivisibleBy<${divisor}>`;
    expect.extend({ toBeDivisibleBy });
    test("x", () => { expect({ n: 7 }).toEqual({ n: (expect as any).toBeDivisibleBy(3) }); });

    Before: - "n": [object Object]toBeDivisibleBy [ / - 3, / - ],. After: - "n": toBeDivisibleBy [ / - 3, / - ],.

  • The first version of this PR made the hook work (call fn_value instead of matcher_fn, return true). A self-review found that this enables an undocumented, untyped, Bun-only hook that nobody has asked for, and that the enabled form loses the not and promise flags, because custom_print never read them and the hook had no way to. Deleting the dead path is the smaller change and keeps no half-designed contract. If a maintainer wants a custom print form for expect.extend matchers, the complete version needs a not/promise decision, a toAsymmetricMatcher? member on CustomMatcher in packages/bun-types/test.d.ts, and a docs entry. That can be its own PR.

  • git show 90654143bb:src/bun.js/test/expect.zig shows the Zig customPrint with the same call target (matcher_fn.callWithThis) and return false. The Zig instance-level toAsymmetricMatcher method was never exposed in jest.classes.ts, and the Rust port does not have it.

  • toAsymmetricMatcher has zero hits in docs/, packages/bun-types/, and src/js/. No package in test/node_modules assigns the property to a matcher function.

  • The test lives in its own Bun-only file, next to expect-extend-asymmetric-match-throw.test.ts and expect-extend-matcher-utils-throw.test.ts. expect-extend.test.js also runs under Jest and Vitest, where the default form differs, and its 10000-iteration GC test runs close to the 5 s limit on debug ASAN builds (test: scale expect-extend intensive-usage iterations down on debug builds #39359).

  • Aligning Bun's default form with Jest's [not.]name<args> is a possible follow-up. It changes .snap files that hold custom property matchers, so it is out of scope here.


[review] gate passed · iteration 1 · 3 files touched

fails on main (without fix)
ASAN without fix: 1 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/test/expect-extend-asymmetric-print.test.ts
bun test v1.4.1 (4448a2e21)

test/js/bun/test/expect-extend-asymmetric-print.test.ts:
25 |   try {
26 |     expect({ n: 7 }).toEqual({ n: anyExpect._toBeDivisibleByHooked(3) });
27 |   } catch (err) {
28 |     message = Bun.stripANSI((err as Error).message);
29 |   }
30 |   expect(message).toContain('"n": _toBeDivisibleByHooked [');
                       ^
error: expect(received).toContain(expected)

Expected to contain: "\"n\": _toBeDivisibleByHooked ["
Received: "expect(received).toEqual(expected)\n\n  {\n-   \"n\": [object Object]_toBeDivisibleByHooked [\n-     3,\n-   ],\n+   \"n\": 7,\n  }\n\n- Expected  - 3\n+ Received  + 1\n"

      at <anonymous> (/workspace/bun/test/js/bun/test/expect-extend-asymmetric-print.test.ts:30:19)
(fail) ignores a toAsymmetricMatcher property on the matcher function [49.77ms]

 0 pass
 1 fail
 2 expect() calls
Ran 1 test across 1 file. [2.32s]
error: script "bd" exited with code 1
__F:1:S:0

release without fix: all passed
bun test v1.4.1-canary.1 (35a7c23fa)

test/js/bun/test/expect-extend-asymmetric-print.test.ts:
(pass) ignores a toAsymmetricMatcher property on the matcher function [0.33ms]

 1 pass
 0 fail
 8 expect() calls
Ran 1 test across 1 file. [104.00ms]
__F:0:S:0
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/test/expect-extend-asymmetric-print.test.ts
bun test v1.4.1 (4448a2e21)

test/js/bun/test/expect-extend-asymmetric-print.test.ts:
(pass) ignores a toAsymmetricMatcher property on the matcher function [95.95ms]

 1 pass
 0 fail
 8 expect() calls
Ran 1 test across 1 file. [2.15s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 786ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/37] gen generated_host_exports.rs
generated_host_exports.rs: 120 exports (host=5, lazy=10, generic=105, rust=0); 243 extern-C blocks audited
[2/37] gen JSBuffer.lut.h
Generating /workspace/bun/build/release/codegen/JSBuffer.lut.h from /workspace/bun/src/jsc/bindings/JSBuffer.cpp
[3/37] gen cpp.rs (cppbind)
[4/37] gen ZigGeneratedClasses.{cpp,h,rs}
Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts
  - ResolveMessage (15 fields)
  - BuildMessage (10 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts
  - Archive (4 fields, 1 class fields)
Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts
  - ResourceUsage (8 fields)
  - Subprocess (20 fields)
Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts
  - CronJob (5 fields)
Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts
  - FileSystemRouter (5 fields)
  - FrameworkFileSystemRouter (2 fields)
  - MatchedRoute (8 fields)
Found 1 classes from /wor
... (truncated)
diff hotspot
src/runtime/test_runner/expect.rs                  | 69 ++--------------------
 src/runtime/test_runner/pretty_format.rs           | 49 ++++++---------
 .../test/expect-extend-asymmetric-print.test.ts    | 38 ++++++++++++
 3 files changed, 62 insertions(+), 94 deletions(-)

gate history · 2 passed · 1 rejected · iteration 1

evidence per changed file
file                                                     reads  edits  tests
src/runtime/test_runner/expect.rs                            6      3      0
src/runtime/test_runner/pretty_format.rs                     2      1      0
test/js/bun/test/expect-extend-asymmetric-print.test.ts      0      1      0

@coderabbitai

coderabbitai Bot commented Aug 22, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 59498feb-dfcf-4abd-82e1-91a1627c614c

📥 Commits

Reviewing files that changed from the base of the PR and between abe2ad4 and 5b99a07.

📒 Files selected for processing (3)
  • src/runtime/test_runner/expect.rs
  • src/runtime/test_runner/pretty_format.rs
  • test/js/bun/test/expect-extend.test.js

Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review.


Walkthrough

Changes

The custom asymmetric matcher formatter no longer calls custom_print. It uses cached matcher data and adds regression coverage for default and negated stringification.

Asymmetric matcher formatting

Layer / File(s) Summary
Default asymmetric matcher formatting
src/runtime/test_runner/expect.rs, src/runtime/test_runner/pretty_format.rs
The implementation removes custom stringification and formats matchers from cached flags, arguments, and matcher names.
Formatting regression coverage
test/js/bun/test/expect-extend.test.js
The test verifies default and negated formatting and confirms that the custom hook is not called.

Suggested reviewers: dylan-conway, jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: removing the unused custom matcher print hook.
Description check ✅ Passed The description explains the problem, fix, background, and verification details, although it uses headings different from the template.

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 22, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 11:35 PM PT - Aug 23rd, 2026

✅ @robobun, your commit f004356a9697380cb496e805774f1f08ed38b4f5 passed in Build #104650! 🎉


🧪   To try this PR locally:

bunx bun-pr 40050

That installs a local version of the PR into your bun-40050 executable, so you can run:

bun-40050 --bun

@robobun

robobun commented Aug 22, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: reworked after self-review, ready for review.

Reproduced on bun 1.4.0 and on main (b30c6ff) with a matcher that carries a toAsymmetricMatcher property. A failing toEqual printed "n": [object Object]toBeDivisibleBy [ 3, ].

The first version of this PR made the hook work. A self-review found that this enables an undocumented, untyped, Bun-only hook that has never worked and that would print expect.not.x(3) and expect.x(3) identically. The PR now deletes the dead custom_print path instead. The diff line prints the default form, "n": toBeDivisibleBy [ 3, ], with no [object Object]. This matches Jest, which ignores a toAsymmetricMatcher property on the matcher function.

Test: test/js/bun/test/expect-extend-asymmetric-print.test.ts. It fails on stock bun and passes with this change.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — two-line fix that makes custom_print call the toAsymmetricMatcher hook it already looked up (instead of the matcher itself) and return true so the default form is suppressed.

What was reviewed

  • custom_print in expect.rs: fn_value is the resolved hook, already type-checked as a function; the call target and return value were the only defects. Error paths (throwing hook, non-function hook, missing captured args) still fall through to Ok(false).
  • The sole caller at pretty_format.rs:2911 passes dont_throw = true and gates the default print on !printed, matching the new Ok(true).
  • New tests cover the hook string, this/args binding, throwing-hook fallback, and non-function fallback; correctly guarded under if (isBun) in the existing expect-extend.test.js.
Extended reasoning...

Overview

Two-line fix in ExpectCustomAsymmetricMatcher::custom_print (src/runtime/test_runner/expect.rs). The function already looked up matcher_fn.toAsymmetricMatcher, checked it was a function, and collected the captured args — but then called matcher_fn (the matcher itself, which returns {pass, message}) instead of fn_value (the hook), and fell through to Ok(false) so the caller printed the default <name> [args] form after the [object Object] string. The PR swaps the call target to fn_value and adds return Ok(true) after the write. The surrounding code makes the intent unambiguous; this is fixing a copy/typo that has existed since the original Zig implementation.

Four new tests are added to test/js/bun/test/expect-extend.test.js under if (isBun) (the hook is Bun-specific), covering: the hook's string replaces the default form (including via this.utils.stringify); the hook receives the matcher instance as this and the captured args; a throwing hook falls back to the default form via maybe_clear with dont_throw = true; a non-function toAsymmetricMatcher property is ignored.

Security risks

None. This is display-only formatting of test failure messages inside bun:test. No parsing of untrusted input, no filesystem/network, no changes to matching semantics.

Level of scrutiny

Low. The change is tiny, mechanical, and the intent is fully corroborated by the surrounding code (the hook lookup, the is_function() check, and the args collection were all already there — only the call target and return value were wrong). I confirmed the sole caller at pretty_format.rs:2911 passes dont_throw = true and branches on the returned bool exactly as the fix expects. All existing error paths (maybe_clear on hook lookup, iterator, call, and to_bun_string) are unchanged and still return Ok(false) to trigger the default form.

Other factors

Tests are placed in the existing file per convention, gated to Bun since Jest/Vitest don't have this hook, and use direct assertions on the failure message rather than snapshots (appropriate here — the assertions are specific and negative checks like not.toContain("[object Object]") verify the exact bug is gone). The PR description documents that 3 of the 4 new tests fail on stock bun and that adjacent expect/pretty-format test files were re-run. No prior human review comments to address.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — two-line fix that makes custom_print call the toAsymmetricMatcher hook it already looked up (instead of the matcher itself) and return true so the caller skips the default form.

What was reviewed:

  • Confirmed the sole caller at pretty_format.rs:2911 passes dont_throw = true and gates the default <name> [args] form on the returned bool, so the throwing-hook and non-function paths still fall through correctly.
  • Checked that this_value (the matcher instance) and the captured args were already being collected for this call — only the call target and return value changed.
  • New tests cover the happy path, this/args binding, throwing hook, and non-function property; gated under isBun since the hook is Bun-specific and the file also runs under Jest/Vitest.
Extended reasoning...

Overview

Two-line change in ExpectCustomAsymmetricMatcher::custom_print (src/runtime/test_runner/expect.rs): swap the call target from matcher_fn to fn_value (the toAsymmetricMatcher property that was already looked up and type-checked as a function), and return Ok(true) after writing the result. The surrounding code — property lookup, function check, captured-args iteration, exception clearing via maybe_clear — is unchanged. Four new tests in test/js/bun/test/expect-extend.test.js under an if (isBun) block.

Security risks

None. This is diff-formatting output for failing test assertions. No untrusted input reaches an allocation size, syscall, or privilege boundary. The hook's return value goes through to_bun_string with the existing exception-clearing path.

Level of scrutiny

Low. The bug is self-evident from reading the function: it fetches fn_value = matcher_fn.get("toAsymmetricMatcher"), checks fn_value.js_type().is_function(), collects args, then called matcher_fn instead of fn_value and returned false regardless. The PR description traces this back to the original Zig implementation having the same defect, so the hook has never worked — no behavior anyone relies on is changing. I read the caller at pretty_format.rs:2911 and confirmed it treats true as "printed, skip default" and passes dont_throw = true, matching what the third and fourth tests assert.

Other factors

Test coverage is thorough for a formatting fix: it asserts the hook's string appears verbatim, the default form and [object Object] do not, this is the matcher instance, args are the captured args, a throwing hook falls back cleanly (exception message not leaked into output), and a non-function toAsymmetricMatcher is ignored. Tests strip ANSI with Bun.stripANSI before substring assertions, avoiding color-code brittleness. The this.utils.stringify sub-check confirms the same formatter path is used by matcher utils. The isBun gate keeps the file runnable under Jest/Vitest as documented at the top of the file. No outstanding reviewer comments.

@robobun
robobun force-pushed the farm/fbb9d38b/custom-asymmetric-matcher-print branch from 4bc5ede to 5b99a07 Compare August 22, 2026 09:18
Comment thread src/runtime/test_runner/expect.rs Resolved
@robobun robobun changed the title bun:test: print a custom asymmetric matcher with its toAsymmetricMatcher hook bun:test: remove the dead toAsymmetricMatcher print hook for custom matchers Aug 22, 2026
Comment thread src/runtime/test_runner/expect.rs Resolved
@robobun
robobun force-pushed the farm/fbb9d38b/custom-asymmetric-matcher-print branch from 5b99a07 to 35a7c23 Compare August 24, 2026 04:47

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — my earlier note about the title/description mismatch has been addressed; both now correctly describe the deletion of custom_print/maybe_clear.

What was reviewed

  • Confirmed custom_print was dead-by-construction: it called matcher_fn instead of fn_value and always returned false, so the only observable effect was the stray [object Object] prefix — no user-relied-upon behavior is being removed.
  • Verified pretty_format.rs change is a pure de-indent of the pre-existing default-form branch; not and promise prefixes are preserved.
  • Grep'd src/ for remaining references to custom_print, maybe_clear, toAsymmetricMatcher — none.
  • The deletion also removes a clear_exception() call and an .expect("unreachable") that could panic on writer I/O error.
Extended reasoning...

Overview

This PR deletes ExpectCustomAsymmetricMatcher::custom_print and its private helper maybe_clear from src/runtime/test_runner/expect.rs, removes the sole call site in src/runtime/test_runner/pretty_format.rs (de-indenting the default-form branch that was gated on if !printed), trims the R-2 re-entrancy comment to drop the reference to custom_print, and adds one regression test in test/js/bun/test/expect-extend.test.js locking in that a toAsymmetricMatcher property on a matcher function is ignored and that the diff output no longer contains [object Object].

Security risks

None. This is test-runner output formatting. The deletion removes a user-JS re-entry point from inside the pretty-formatter and a global_this.clear_exception() call — both are net improvements per REVIEW.md ("Never clearException()", minimize re-entrancy inside formatters).

Level of scrutiny

Low-to-medium. The removed function was demonstrably broken since inception (called matcher_fn instead of the looked-up fn_value, then unconditionally returned Ok(false) so the caller always fell through to the default form anyway). The only user-observable behavior it produced was prepending [object Object] to the diff line when a matcher function happened to carry a toAsymmetricMatcher property — undocumented, untyped, and with no Jest/Vitest analogue on the matcher function (Jest generates it on the instance). The pretty_format.rs hunk is byte-identical to the previous if !printed body, just outdented; I read both against the current file and confirmed no logic drift. Grep confirms no remaining references to the deleted symbols anywhere in src/.

Other factors

My previous review on this PR flagged only a metadata mismatch (title/description described a fix, diff was a deletion). The author rewrote both; the current title and body accurately name the deletions per REVIEW.md's dead-code rule, and the Notes section records why the earlier "make it work" approach was abandoned (would print expect.not.x and expect.x identically, and would ship an untyped/undocumented Bun-only hook). The github-actions comment-cop flag on the R-2 comment was correctly rebutted — the comment is pre-existing convention and this PR only removed the stale custom_print mention. The new test fails on stock bun (per the gate evidence in the description) and covers both the toEqual diff path and the this.utils.stringify path, plus the not prefix. CI build #104614 is running on the current commit.

…atchers

ExpectCustomAsymmetricMatcher::custom_print looked for a toAsymmetricMatcher
function on the matcher, then called the matcher itself in its place and
returned false. A failing toEqual with a custom asymmetric matcher that
carried that property printed "[object Object]" followed by the default
"<name> [args]" form. The same defects exist in the Zig original, so the
hook has never produced correct output. It has no types and no docs.

Delete custom_print and maybe_clear, and print the default form directly.
This matches Jest, which ignores a toAsymmetricMatcher property on the
matcher function, and removes a user JS re-entry from the formatter.
@robobun
robobun force-pushed the farm/fbb9d38b/custom-asymmetric-matcher-print branch from 35a7c23 to f004356 Compare August 24, 2026 05:11

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant